Automated Test: rest-serializer-enhancement-post #324
Conversation
📝 WalkthroughWalkthroughThis pull request migrates the embeddable hosts configuration from site settings to a database-backed model. It introduces a new EmbeddableHost model with validation and host lookup methods, adds admin UI for managing hosts via Ember components and routes, creates REST controllers and serializers for API operations, and updates existing embed integration code to use the new model instead of settings. A database migration transfers existing configuration data and removes deprecated site settings. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 Fix all issues with AI agents
In `@app/assets/javascripts/admin/components/embeddable-host.js.es6`:
- Around line 43-51: The delete() action calls this.get('host').destroyRecord()
without handling failures; add a .catch() after destroyRecord() (or use
.then(...).catch(...)) to handle server errors: on success keep the existing
this.sendAction('deleteHost', this.get('host')), on failure notify the user
(e.g., show a bootbox.alert or I18n.t message) and avoid sending deleteHost so
the UI stays consistent; reference the delete() method, bootbox.confirm,
destroyRecord() promise, and the sendAction('deleteHost', this.get('host')) call
when making the change.
In `@app/assets/javascripts/discourse/models/store.js.es6`:
- Around line 192-201: Guard the plural hydration before using obj[k].map by
checking Array.isArray(obj[k]) (or coercing to an empty array) so it doesn't
throw for null/non-array; call self._lookupSubType(subType, type, id, root) for
each id, filter out unresolved/null results, assign obj[self.pluralize(subType)]
= hydratedFiltered || []; and only delete obj[k] if hydratedFiltered has at
least one resolved item (preserve the original id list when nothing resolves).
Ensure you reference m, subType, obj[k], self._lookupSubType and
self.pluralize(subType) when making the change.
In `@app/controllers/admin/embeddable_hosts_controller.rb`:
- Around line 9-12: The update and destroy actions fetch records with
EmbeddableHost.where(id: params[:id]).first without nil checks; change these to
use EmbeddableHost.find_by(id: params[:id]) (or keep where(...).first) and add a
guard that handles a missing record (e.g. render head :not_found or redirect
with an error) before calling save_host or destroy; update both the update
method and the destroy method so they check the found_host variable (or host)
and return early if nil to avoid NoMethodError.
In `@app/models/embeddable_host.rb`:
- Around line 5-8: The before_validation callback currently calls self.host.sub!
which will raise NoMethodError if host is nil; update the callback in
embeddable_host.rb to guard against nil (e.g., return unless host or unless
host.present?) before calling self.host.sub! so the substitutions only run when
host is a non-nil string, keeping the existing modifications (remove protocol
and path) intact.
- Line 2: The TLD length is too restrictive in the host validation: update the
regex used in validates_format_of :host to allow longer TLDs (e.g., replace the
`[a-z]{2,5}` portion with a pattern allowing up to 63 characters like
`[a-z]{2,63}` or `[a-z]{2,}`) so valid long TLDs (e.g., .museum, .technology)
are accepted; keep the rest of the existing pattern (including the port and path
groups and the case-insensitive flag) unchanged.
In `@config/locales/client.en.yml`:
- Around line 2494-2501: The embedding translation keys currently live under
admin_js.badges.embedding but the admin nav template expects
admin.embedding.title; to fix, relocate or duplicate the keys from
admin_js.badges.embedding (embedding.confirm_delete, embedding.title,
embedding.host, embedding.edit, embedding.category, embedding.add_host) into the
admin_js.admin.embedding namespace (or change the template to reference
admin_js.badges.embedding.*) so the admin nav can find admin.embedding.title and
related strings.
In `@db/migrate/20150818190757_create_embeddable_hosts.rb`:
- Around line 9-16: The migration accesses execute(...)[0]['id'] and
execute(...)[0]['value'] without nil checks; change both lookups to capture the
query result into a variable (e.g., res = execute(...)) and only read
res[0]['id'] or res[0]['value'] when res and res[0] exist (or res.any?),
otherwise fall back to the alternate lookup or leave category_id nil as the
embeddable_hosts pattern does; update the assignments around category_id and the
uncategorized_category_id query accordingly to avoid NoMethodError.
In `@spec/fabricators/category_fabricator.rb`:
- Around line 1-4: The fabricator definitions are in the wrong files:
spec/fabricators/category_fabricator.rb currently defines
Fabricator(:embeddable_host) while
spec/fabricators/embeddable_host_fabricator.rb defines Fabricator(:category);
fix this by swapping the contents of the two files (or renaming the files to
match their definitions) so that the file containing Fabricator(:category) is
named/located as the category fabricator and the file containing
Fabricator(:embeddable_host) is named/located as the embeddable_host fabricator.
In `@spec/fabricators/embeddable_host_fabricator.rb`:
- Around line 23-26: In the after_build block of the embeddable_host_fabricator,
avoid calling cat.update! (which persists) and instead assign the attribute
directly (e.g., cat.read_restricted = true); also guard against a nil transient
group by checking transients[:group] (or using if transients[:group]) before
calling cat.category_groups.build(..., group_id: transients[:group].id,
permission_type: CategoryGroup.permission_types[:full]) so you only build the
association when the group is present.
In `@spec/models/topic_spec.rb`:
- Line 1407: Fix the typo in the RSpec describe block string: rename the context
label "with an emeddable host" to "with an embeddable host" in the spec (the
describe block containing that exact string) so the test description reads
correctly.
In `@test/javascripts/helpers/create-pretender.js.es6`:
- Around line 226-229: The GET handler for '/fruits/:id' currently always
returns fruits[0]; update the this.get('/fruits/:id', ...) handler to read the
requested id from request.params.id, find the matching item in the fruits array
(by id), and return response({ __rest_serializer: "1", fruit, farmers, colors })
for the found fruit; if no match is found, return a 404 response. Ensure you
reference the existing route handler (this.get('/fruits/:id', ...)), the fruits
fixture array, the request.params.id lookup, and the response(...) helper when
making the change.
In `@test/javascripts/models/store-test.js.es6`:
- Around line 111-117: The test is requesting fruit id 2 but asserts colors with
ids 1 and 2 (apple); change the request to ask for the apple fixture by
replacing store.find('fruit', 2) with store.find('fruit', 1) so the subsequent
assertions (fruitCols.length and fruitCols[0].get('id') /
fruitCols[1].get('id')) match the returned colors; alternatively, if you prefer
to test id 2, update those assert.equal expectations to the actual color ids for
fruit id 2 instead of 1 and 2.
🧹 Nitpick comments (10)
test/javascripts/models/store-test.js.es6 (1)
114-117: Use Ember array accessors forcolorsinstead of bracket indexing.If
colorsresolves to an Ember.Array/ManyArray,fruitCols[0]is not guaranteed. PreferobjectAtfor consistency with other assertions in this file.♻️ Suggested update
- assert.equal(fruitCols[0].get('id'), 1); - assert.equal(fruitCols[1].get('id'), 2); + assert.equal(fruitCols.objectAt(0).get('id'), 1); + assert.equal(fruitCols.objectAt(1).get('id'), 2);- assert.equal(fruitCols[0].get('id'), 1); - assert.equal(fruitCols[1].get('id'), 2); + assert.equal(fruitCols.objectAt(0).get('id'), 1); + assert.equal(fruitCols.objectAt(1).get('id'), 2);Also applies to: 129-132
spec/controllers/admin/embedding_controller_spec.rb (1)
3-9: Remove extra empty lines to satisfy RuboCop.Static analysis flagged extra empty lines at the beginning and end of the block body.
Proposed fix
describe Admin::EmbeddingController do - it "is a subclass of AdminController" do expect(Admin::EmbeddingController < Admin::AdminController).to eq(true) end - endspec/controllers/admin/embeddable_hosts_controller_spec.rb (1)
3-9: Remove extra empty lines to satisfy RuboCop.Static analysis flagged extra empty lines at the beginning and end of the block body.
Proposed fix
describe Admin::EmbeddableHostsController do - it "is a subclass of AdminController" do expect(Admin::EmbeddableHostsController < Admin::AdminController).to eq(true) end - endapp/assets/javascripts/admin/routes/admin-embedding.js.es6 (1)
6-8: Consider callingthis._super()insetupController.When overriding
setupControllerin Ember routes, it's a best practice to callthis._super(controller, model)to ensure the parent class behavior (like settingcontroller.model) is preserved. This prevents potential issues if future code expects the default model assignment.Suggested fix
setupController(controller, model) { + this._super(controller, model); controller.set('embedding', model); }app/serializers/embeddable_host_serializer.rb (1)
4-14: Remove redundant getter methods.These explicit getter methods are unnecessary boilerplate. ActiveModelSerializers automatically delegates attribute access to the underlying object when no custom method is defined. The serializer will function identically with just the
attributesdeclaration.Suggested simplification
class EmbeddableHostSerializer < ApplicationSerializer attributes :id, :host, :category_id - - def id - object.id - end - - def host - object.host - end - - def category_id - object.category_id - end endapp/serializers/embedding_serializer.rb (1)
5-7: Remove redundantidmethod.Same as in
EmbeddableHostSerializer, this explicit getter is unnecessary. ActiveModelSerializers automatically delegates toobject.id.Suggested simplification
class EmbeddingSerializer < ApplicationSerializer attributes :id has_many :embeddable_hosts, serializer: EmbeddableHostSerializer, embed: :ids - - def id - object.id - end endspec/models/embeddable_host_spec.rb (1)
4-5: Minor: Extra empty lines flagged by RuboCop.Static analysis detected extra empty lines at the beginning of the outer block (line 4-5) and at the end of nested blocks (lines 37-38, 39-40). Consider removing these for consistency with
Layout/EmptyLinesAroundBlockBodyconvention.♻️ Proposed formatting fix
describe EmbeddableHost do - it "trims http" doexpect(EmbeddableHost.host_allowed?('http://discourse.org')).to eq(true) end - end - endAlso applies to: 37-40
app/models/embeddable_host.rb (1)
10-18: Consider using explicit rescue block instead of modifier form.The inline
rescue nilat line 11 catches all exceptions, which could mask unexpected errors. RuboCop recommends avoiding the modifier form.♻️ Proposed refactor
def self.record_for_host(host) - uri = URI(host) rescue nil + begin + uri = URI(host) + rescue URI::InvalidURIError + return false + end return false unless uri.present? host = uri.host return false unless host.present? - where("lower(host) = ?", host).first + where("lower(host) = ?", host.downcase).first endNote: Also added
.downcaseto ensure consistent case comparison sinceuri.hostmay preserve case.app/controllers/admin/embeddable_hosts_controller.rb (1)
22-32: Consider using strong parameters.While the controller is admin-only, using strong parameters would provide an additional layer of protection against mass assignment vulnerabilities.
♻️ Proposed refactor
+ private + + def embeddable_host_params + params.require(:embeddable_host).permit(:host, :category_id) + end + protected def save_host(host) - host.host = params[:embeddable_host][:host] - host.category_id = params[:embeddable_host][:category_id] + host.host = embeddable_host_params[:host] + host.category_id = embeddable_host_params[:category_id] host.category_id = SiteSetting.uncategorized_category_id if host.category_id.blank?app/controllers/admin/embedding_controller.rb (1)
9-11:updateaction doesn't modify the embedding resource.The
updateaction simply renders the embedding without any modifications. This is intentional: the actual host updates are handled throughEmbeddableHostsController. Consider adding a comment to clarify that the embedding itself is read-only and that individual hosts are updated separately.
| delete() { | ||
| bootbox.confirm(I18n.t('admin.embedding.confirm_delete'), (result) => { | ||
| if (result) { | ||
| this.get('host').destroyRecord().then(() => { | ||
| this.sendAction('deleteHost', this.get('host')); | ||
| }); | ||
| } | ||
| }); | ||
| }, |
There was a problem hiding this comment.
Missing error handling in delete action.
The destroyRecord() call doesn't have error handling. If the server request fails, the user won't be notified and the UI state may become inconsistent.
Proposed fix to add error handling
delete() {
bootbox.confirm(I18n.t('admin.embedding.confirm_delete'), (result) => {
if (result) {
this.get('host').destroyRecord().then(() => {
this.sendAction('deleteHost', this.get('host'));
- });
+ }).catch(popupAjaxError);
}
});
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| delete() { | |
| bootbox.confirm(I18n.t('admin.embedding.confirm_delete'), (result) => { | |
| if (result) { | |
| this.get('host').destroyRecord().then(() => { | |
| this.sendAction('deleteHost', this.get('host')); | |
| }); | |
| } | |
| }); | |
| }, | |
| delete() { | |
| bootbox.confirm(I18n.t('admin.embedding.confirm_delete'), (result) => { | |
| if (result) { | |
| this.get('host').destroyRecord().then(() => { | |
| this.sendAction('deleteHost', this.get('host')); | |
| }).catch(popupAjaxError); | |
| } | |
| }); | |
| }, |
🤖 Prompt for AI Agents
In `@app/assets/javascripts/admin/components/embeddable-host.js.es6` around lines
43 - 51, The delete() action calls this.get('host').destroyRecord() without
handling failures; add a .catch() after destroyRecord() (or use
.then(...).catch(...)) to handle server errors: on success keep the existing
this.sendAction('deleteHost', this.get('host')), on failure notify the user
(e.g., show a bootbox.alert or I18n.t message) and avoid sending deleteHost so
the UI stays consistent; reference the delete() method, bootbox.confirm,
destroyRecord() promise, and the sendAction('deleteHost', this.get('host')) call
when making the change.
| const m = /(.+)\_id(s?)$/.exec(k); | ||
| if (m) { | ||
| const subType = m[1]; | ||
| const hydrated = self._lookupSubType(subType, type, obj[k], root); | ||
| if (hydrated) { | ||
| obj[subType] = hydrated; | ||
|
|
||
| if (m[2]) { | ||
| const hydrated = obj[k].map(function(id) { | ||
| return self._lookupSubType(subType, type, id, root); | ||
| }); | ||
| obj[self.pluralize(subType)] = hydrated || []; | ||
| delete obj[k]; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and view the relevant file
fd -e es6 store.jsRepository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 185
🏁 Script executed:
# Get the file and examine the _hydrateEmbedded function context
cat -n app/assets/javascripts/discourse/models/store.js.es6 | head -250 | tail -100Repository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 3591
Guard plural hydration against non-arrays and preserve ids when nothing resolves.
obj[k].map(...) will throw if *_ids is null or non-array. Additionally, this branch unconditionally deletes the id list even when nothing hydrates, unlike the singular path (which only deletes on successful hydration), creating a data loss risk. Add an array guard, filter unresolved ids, and conditionally delete.
🛠️ Suggested guard + preserve ids when nothing resolves
if (m[2]) {
- const hydrated = obj[k].map(function(id) {
+ const ids = Array.isArray(obj[k]) ? obj[k] : [];
+ const hydrated = ids.map(function(id) {
return self._lookupSubType(subType, type, id, root);
- });
- obj[self.pluralize(subType)] = hydrated || [];
- delete obj[k];
+ }).filter(Boolean);
+
+ if (ids.length === 0 || hydrated.length > 0) {
+ obj[self.pluralize(subType)] = hydrated;
+ delete obj[k];
+ }
} else {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const m = /(.+)\_id(s?)$/.exec(k); | |
| if (m) { | |
| const subType = m[1]; | |
| const hydrated = self._lookupSubType(subType, type, obj[k], root); | |
| if (hydrated) { | |
| obj[subType] = hydrated; | |
| if (m[2]) { | |
| const hydrated = obj[k].map(function(id) { | |
| return self._lookupSubType(subType, type, id, root); | |
| }); | |
| obj[self.pluralize(subType)] = hydrated || []; | |
| delete obj[k]; | |
| const m = /(.+)\_id(s?)$/.exec(k); | |
| if (m) { | |
| const subType = m[1]; | |
| if (m[2]) { | |
| const ids = Array.isArray(obj[k]) ? obj[k] : []; | |
| const hydrated = ids.map(function(id) { | |
| return self._lookupSubType(subType, type, id, root); | |
| }).filter(Boolean); | |
| if (ids.length === 0 || hydrated.length > 0) { | |
| obj[self.pluralize(subType)] = hydrated; | |
| delete obj[k]; | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@app/assets/javascripts/discourse/models/store.js.es6` around lines 192 - 201,
Guard the plural hydration before using obj[k].map by checking
Array.isArray(obj[k]) (or coercing to an empty array) so it doesn't throw for
null/non-array; call self._lookupSubType(subType, type, id, root) for each id,
filter out unresolved/null results, assign obj[self.pluralize(subType)] =
hydratedFiltered || []; and only delete obj[k] if hydratedFiltered has at least
one resolved item (preserve the original id list when nothing resolves). Ensure
you reference m, subType, obj[k], self._lookupSubType and
self.pluralize(subType) when making the change.
| def update | ||
| host = EmbeddableHost.where(id: params[:id]).first | ||
| save_host(host) | ||
| end |
There was a problem hiding this comment.
Missing nil checks for record lookups.
Both update and destroy actions use where(id: params[:id]).first without checking if the record exists. This will cause NoMethodError if the record isn't found.
🐛 Proposed fix using find_by with guard
def update
- host = EmbeddableHost.where(id: params[:id]).first
+ host = EmbeddableHost.find_by(id: params[:id])
+ return render_json_error(I18n.t('not_found')) unless host
save_host(host)
end
def destroy
- host = EmbeddableHost.where(id: params[:id]).first
+ host = EmbeddableHost.find_by(id: params[:id])
+ return render_json_error(I18n.t('not_found')) unless host
host.destroy
render json: success_json
endAlso applies to: 14-18
🤖 Prompt for AI Agents
In `@app/controllers/admin/embeddable_hosts_controller.rb` around lines 9 - 12,
The update and destroy actions fetch records with EmbeddableHost.where(id:
params[:id]).first without nil checks; change these to use
EmbeddableHost.find_by(id: params[:id]) (or keep where(...).first) and add a
guard that handles a missing record (e.g. render head :not_found or redirect
with an error) before calling save_host or destroy; update both the update
method and the destroy method so they check the found_host variable (or host)
and return early if nil to avoid NoMethodError.
| @@ -0,0 +1,24 @@ | |||
| class EmbeddableHost < ActiveRecord::Base | |||
| validates_format_of :host, :with => /\A[a-z0-9]+([\-\.]{1}[a-z0-9]+)*\.[a-z]{2,5}(:[0-9]{1,5})?(\/.*)?\Z/i | |||
There was a problem hiding this comment.
TLD validation may reject valid domains.
The regex limits TLDs to 2-5 characters ([a-z]{2,5}), which excludes valid TLDs like .museum, .technology, .international, etc. Consider relaxing this constraint.
♻️ Suggested regex adjustment
- validates_format_of :host, :with => /\A[a-z0-9]+([\-\.]{1}[a-z0-9]+)*\.[a-z]{2,5}(:[0-9]{1,5})?(\/.*)?\Z/i
+ validates_format_of :host, :with => /\A[a-z0-9]+([\-\.]{1}[a-z0-9]+)*\.[a-z]{2,}(:[0-9]{1,5})?(\/.*)?\Z/i📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| validates_format_of :host, :with => /\A[a-z0-9]+([\-\.]{1}[a-z0-9]+)*\.[a-z]{2,5}(:[0-9]{1,5})?(\/.*)?\Z/i | |
| validates_format_of :host, :with => /\A[a-z0-9]+([\-\.]{1}[a-z0-9]+)*\.[a-z]{2,}(:[0-9]{1,5})?(\/.*)?\Z/i |
🤖 Prompt for AI Agents
In `@app/models/embeddable_host.rb` at line 2, The TLD length is too restrictive
in the host validation: update the regex used in validates_format_of :host to
allow longer TLDs (e.g., replace the `[a-z]{2,5}` portion with a pattern
allowing up to 63 characters like `[a-z]{2,63}` or `[a-z]{2,}`) so valid long
TLDs (e.g., .museum, .technology) are accepted; keep the rest of the existing
pattern (including the port and path groups and the case-insensitive flag)
unchanged.
| before_validation do | ||
| self.host.sub!(/^https?:\/\//, '') | ||
| self.host.sub!(/\/.*$/, '') | ||
| end |
There was a problem hiding this comment.
Potential NoMethodError if host is nil.
The before_validation callback calls self.host.sub! which will raise NoMethodError if host is nil. Consider adding a guard clause.
🐛 Proposed fix
before_validation do
+ return if self.host.blank?
self.host.sub!(/^https?:\/\//, '')
self.host.sub!(/\/.*$/, '')
end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| before_validation do | |
| self.host.sub!(/^https?:\/\//, '') | |
| self.host.sub!(/\/.*$/, '') | |
| end | |
| before_validation do | |
| return if self.host.blank? | |
| self.host.sub!(/^https?:\/\//, '') | |
| self.host.sub!(/\/.*$/, '') | |
| end |
🤖 Prompt for AI Agents
In `@app/models/embeddable_host.rb` around lines 5 - 8, The before_validation
callback currently calls self.host.sub! which will raise NoMethodError if host
is nil; update the callback in embeddable_host.rb to guard against nil (e.g.,
return unless host or unless host.present?) before calling self.host.sub! so the
substitutions only run when host is a non-nil string, keeping the existing
modifications (remove protocol and path) intact.
| Fabricator(:embeddable_host) do | ||
| host "eviltrout.com" | ||
| category | ||
| end |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if category fabricator is defined elsewhere
rg -n "Fabricator\(:category\)" spec/fabricators/Repository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 161
🏁 Script executed:
#!/bin/bash
# Check the contents of both files to understand the naming mismatch
echo "=== category_fabricator.rb ==="
cat spec/fabricators/category_fabricator.rb
echo ""
echo "=== embeddable_host_fabricator.rb ==="
cat spec/fabricators/embeddable_host_fabricator.rbRepository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 832
The fabricator files are completely swapped.
spec/fabricators/category_fabricator.rb contains the Fabricator(:embeddable_host) definition (and should contain the category fabricators), while spec/fabricators/embeddable_host_fabricator.rb contains Fabricator(:category) and its variants (and should contain the embeddable_host fabricator). Swap the contents of these two files to match their filenames, or rename them to reflect their actual content.
🤖 Prompt for AI Agents
In `@spec/fabricators/category_fabricator.rb` around lines 1 - 4, The fabricator
definitions are in the wrong files: spec/fabricators/category_fabricator.rb
currently defines Fabricator(:embeddable_host) while
spec/fabricators/embeddable_host_fabricator.rb defines Fabricator(:category);
fix this by swapping the contents of the two files (or renaming the files to
match their definitions) so that the file containing Fabricator(:category) is
named/located as the category fabricator and the file containing
Fabricator(:embeddable_host) is named/located as the embeddable_host fabricator.
| after_build do |cat, transients| | ||
| cat.update!(read_restricted: true) | ||
| cat.category_groups.build(group_id: transients[:group].id, permission_type: CategoryGroup.permission_types[:full]) | ||
| end |
There was a problem hiding this comment.
Avoid update! in after_build and guard against nil transient.
Two issues in this callback:
update!inafter_buildis incorrect—the record isn't persisted yet, so this either fails or causes a premature save. Use direct assignment instead.transients[:group]may be nil if not provided, causing aNoMethodError.
Proposed fix
after_build do |cat, transients|
- cat.update!(read_restricted: true)
- cat.category_groups.build(group_id: transients[:group].id, permission_type: CategoryGroup.permission_types[:full])
+ cat.read_restricted = true
+ if transients[:group]
+ cat.category_groups.build(group_id: transients[:group].id, permission_type: CategoryGroup.permission_types[:full])
+ end
end🤖 Prompt for AI Agents
In `@spec/fabricators/embeddable_host_fabricator.rb` around lines 23 - 26, In the
after_build block of the embeddable_host_fabricator, avoid calling cat.update!
(which persists) and instead assign the attribute directly (e.g.,
cat.read_restricted = true); also guard against a nil transient group by
checking transients[:group] (or using if transients[:group]) before calling
cat.category_groups.build(..., group_id: transients[:group].id, permission_type:
CategoryGroup.permission_types[:full]) so you only build the association when
the group is present.
| SiteSetting.embed_truncate = false | ||
| expect(topic.expandable_first_post?).to eq(false) | ||
| end | ||
| describe 'with an emeddable host' do |
There was a problem hiding this comment.
Typo: "emeddable" should be "embeddable".
The describe block has a typo in the context name.
✏️ Proposed fix
- describe 'with an emeddable host' do
+ describe 'with an embeddable host' do📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| describe 'with an emeddable host' do | |
| describe 'with an embeddable host' do |
🤖 Prompt for AI Agents
In `@spec/models/topic_spec.rb` at line 1407, Fix the typo in the RSpec describe
block string: rename the context label "with an emeddable host" to "with an
embeddable host" in the spec (the describe block containing that exact string)
so the test description reads correctly.
| this.get('/fruits/:id', function() { | ||
| const fruit = fruits[0]; | ||
|
|
||
| return response({ __rest_serializer: "1", fruit, farmers: [farmers[0]] }); | ||
| return response({ __rest_serializer: "1", fruit, farmers, colors }); | ||
| }); |
There was a problem hiding this comment.
Honor the requested fruit id in the fixture response.
/fruits/:id still returns fruits[0], so the payload id can differ from the requested id (now id 2 in tests). This can hide serializer/store bugs and makes the fixture misleading. Consider selecting by request.params.id and returning 404 if not found.
🔧 Suggested fix
- this.get('/fruits/:id', function() {
- const fruit = fruits[0];
- return response({ __rest_serializer: "1", fruit, farmers, colors });
- });
+ this.get('/fruits/:id', function(request) {
+ const fruitId = parseInt(request.params.id, 10);
+ const fruit = fruits.findBy('id', fruitId);
+ if (!fruit) {
+ return response(404, { error: 'fruit not found' });
+ }
+ return response({ __rest_serializer: "1", fruit, farmers, colors });
+ });🤖 Prompt for AI Agents
In `@test/javascripts/helpers/create-pretender.js.es6` around lines 226 - 229, The
GET handler for '/fruits/:id' currently always returns fruits[0]; update the
this.get('/fruits/:id', ...) handler to read the requested id from
request.params.id, find the matching item in the fruits array (by id), and
return response({ __rest_serializer: "1", fruit, farmers, colors }) for the
found fruit; if no match is found, return a 404 response. Ensure you reference
the existing route handler (this.get('/fruits/:id', ...)), the fruits fixture
array, the request.params.id lookup, and the response(...) helper when making
the change.
| return store.find('fruit', 2).then(function(f) { | ||
| assert.ok(f.get('farmer'), 'it has the embedded object'); | ||
|
|
||
| const fruitCols = f.get('colors'); | ||
| assert.equal(fruitCols.length, 2); | ||
| assert.equal(fruitCols[0].get('id'), 1); | ||
| assert.equal(fruitCols[1].get('id'), 2); |
There was a problem hiding this comment.
Align the requested fruit id with the expected colors.
The test requests fruit id 2, but the expected colors [1,2] correspond to the apple fixture (id 1). This becomes inconsistent once /fruits/:id honors the request param. Either change the request to id 1 or update expectations to match id 2’s colors.
✅ Option A (keep expectations, request apple)
- return store.find('fruit', 2).then(function(f) {
+ return store.find('fruit', 1).then(function(f) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return store.find('fruit', 2).then(function(f) { | |
| assert.ok(f.get('farmer'), 'it has the embedded object'); | |
| const fruitCols = f.get('colors'); | |
| assert.equal(fruitCols.length, 2); | |
| assert.equal(fruitCols[0].get('id'), 1); | |
| assert.equal(fruitCols[1].get('id'), 2); | |
| return store.find('fruit', 1).then(function(f) { | |
| assert.ok(f.get('farmer'), 'it has the embedded object'); | |
| const fruitCols = f.get('colors'); | |
| assert.equal(fruitCols.length, 2); | |
| assert.equal(fruitCols[0].get('id'), 1); | |
| assert.equal(fruitCols[1].get('id'), 2); |
🤖 Prompt for AI Agents
In `@test/javascripts/models/store-test.js.es6` around lines 111 - 117, The test
is requesting fruit id 2 but asserts colors with ids 1 and 2 (apple); change the
request to ask for the apple fixture by replacing store.find('fruit', 2) with
store.find('fruit', 1) so the subsequent assertions (fruitCols.length and
fruitCols[0].get('id') / fruitCols[1].get('id')) match the returned colors;
alternatively, if you prefer to test id 2, update those assert.equal
expectations to the actual color ids for fruit id 2 instead of 1 and 2.
This pull request was automatically created by
@coderabbitai/e2e-reviewer.Batch created pull request.
Summary by CodeRabbit
New Features
Refactor